@claude

Claude finished @iamquang95's task in 2m 33s —— View job

Code Review: feat(eth2util): implement signing

• Read changed files and understand PR scope
• Review Go source for functional equivalence
• Analyze new signing.rs implementation
• Review changes to eth2api and other crates
• Post findings

Overview

This PR ports charon/eth2util/signing/signing.go to Rust, adds message_root() helpers to versioned types, and refactors signeddata.rs to delegate to those helpers. The implementation is generally correct and well-tested.

Issues

1. Performance: Double genesis API call in fetch_domain — extensions.rs:323-342

fetch_domain sequentially calls both fetch_genesis_fork_version() and fetch_genesis_validators_root(), each of which independently calls fetch_genesis_data() → /eth/v1/beacon/genesis. That's two network round-trips to the same endpoint per domain lookup.

// Both of these independently call fetch_genesis_data() → /eth/v1/beacon/genesis
let genesis_fork_version = self.fetch_genesis_fork_version().await?;
let genesis_validators_root = self.fetch_genesis_validators_root().await?;

Fix: call fetch_genesis_data() once and parse both fields.

Fix this →

2. Minor divergence from Go: zero-signature check ordering — signing.rs:142-146

Go's Verify fetches the domain first, then checks for a zero signature. The Rust version checks for zero signature before the async get_data_root call. This is actually an improvement (avoids an unnecessary network call), but it's a behavioral divergence worth documenting if callers rely on specific error precedence.

// Rust: checks zero sig first (faster, but differs from Go order)
if *signature == [0; 96] {
    return Err(SigningError::ZeroSignature);
}
let signing_root = get_data_root(client, domain_name, epoch, message_root).await?;

Not a bug — but if exact parity with Go is required, the order should match. Consider adding a comment explaining the intentional divergence.

3. Minor: compute_signing_root visibility — signing.rs:95

pub(crate) fn compute_signing_root(message_root: Root, domain: Domain) -> Root {

This is called from deposit/types.rs and registration.rs both within pluto_eth2util, so pub(crate) is correct. However, get_data_root and get_domain are pub — if consumers ever need the low-level primitive (e.g., for deposit/registration use cases where there's no async beacon client), they can't access it. Low priority, but worth considering making it pub.

4. Missing test: verify rejects wrong message root — signing.rs:tests

The test suite covers valid sig, zero sig, and wrong pubkey. There is no test for a valid signature produced over a different message root but with the correct pubkey. This is a meaningful negative case.

// Suggested test: sign message_root_a, then call verify with message_root_b
// Should return Err(SigningError::Verification(_))

Fix this →

Positive Observations

• Functional parity: get_domain, get_data_root, verify, and verify_aggregate_and_proof_selection all match the Go implementations correctly, including the EIP-7044 voluntary-exit Capella pinning and the genesis-domain special-case for DomainApplicationBuilder.
• Good refactoring in signeddata.rs: Delegating to message_root() methods on versioned types removes duplicated SSZ hashing logic and is cleaner.
• Test vectors: The compute_signing_root_matches_registration_vector and get_data_root_matches_registration_vector tests pin against known Teku test data, providing strong confidence in correctness.
• SyncContributionAndProof message root: selection_proof_message_root() correctly hashes SyncAggregatorSelectionData { slot, subcommittee_index } — consistent with the spec and the old inline code it replaces.

Summary

The implementation is correct and well-structured. The main actionable items are: (1) eliminating the redundant genesis API call in fetch_domain, (2) optionally documenting the zero-sig check divergence from Go, and (3) adding a test for the wrong-message-root case.